Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

rclc_executor: improve enum type names #379

Merged
merged 6 commits into from
Jun 27, 2023
Merged

rclc_executor: improve enum type names #379

merged 6 commits into from
Jun 27, 2023

Conversation

JanStaschulat
Copy link
Contributor

see issue #378

Thanks for your suggestion to improve the names of the enum type names @gavanderhoorn.

@JanStaschulat JanStaschulat requested a review from pablogs9 June 27, 2023 10:58
@JanStaschulat JanStaschulat self-assigned this Jun 27, 2023
@JanStaschulat
Copy link
Contributor Author

@Mergifyio backport iron humble

@mergify
Copy link
Contributor

mergify bot commented Jun 27, 2023 •
edited
Loading

backport iron humble

✅ Backports have been created

@gavanderhoorn
Copy link

Thanks for addressing #378.

Tiny suggestion: I've seen other maintainers use a common prefix for enum members. This helps discoverability, especially when using an editor with code-completion (ie: start typing PREFIX_ and all values show up).

Using a prefix instead of an _EXECUTOR suffix would align with that practice, but it's completely up to you.

@JanStaschulat
updated type names
e237e34 
Signed-off-by: Jan Staschulat <[email protected]>
@JanStaschulat
improved documentation
f89141a 
Signed-off-by: Jan Staschulat <[email protected]>
pablogs9
pablogs9 approved these changes Jun 27, 2023
View reviewed changes
Copy link
Member

@pablogs9 pablogs9 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a comment, what about prefixing them with RCLC_*** ?

@codecov-commenter
Copy link

codecov-commenter commented Jun 27, 2023 •
edited
Loading

Codecov Report

Merging #379 (c3fddcc) into rolling (4b8670d) will decrease coverage by 0.08%.
The diff coverage is 66.66%.

@@             Coverage Diff             @@
##           rolling     #379      +/-   ##
===========================================
- Coverage    69.63%   69.56%   -0.08%     
===========================================
  Files           16       16              
  Lines         2760     2760              
  Branches       766      766              
===========================================
- Hits          1922     1920       -2     
- Misses         452      453       +1     
- Partials       386      387       +1
Impacted Files Coverage Δ
rclc/src/rclc/executor.c 61.90% <66.66%> (ø)

... and 1 file with indirect coverage changes

@JanStaschulat
added documentation for LET semantics
6bb24e0 
Signed-off-by: Jan Staschulat <[email protected]>
@JanStaschulat
added prefix RCLC_
c3fddcc 
Signed-off-by: Jan Staschulat <[email protected]>
@JanStaschulat JanStaschulat changed the title more descriptive names for executor type names Improve executor_type enum names Jun 27, 2023
@JanStaschulat JanStaschulat changed the title Improve executor_type enum names rclc_executor: improve enum type names Jun 27, 2023
gavanderhoorn
gavanderhoorn approved these changes Jun 27, 2023
View reviewed changes
Copy link

@gavanderhoorn gavanderhoorn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@JanStaschulat JanStaschulat merged commit 14c2c41 into rolling Jun 27, 2023
@JanStaschulat JanStaschulat deleted the feature/better-type-names branch June 27, 2023 13:12
mergify bot pushed a commit that referenced this pull request Jun 27, 2023
@JanStaschulat @mergify
rclc_executor: improve enum type names (#379)
0b16e6c 
(cherry picked from commit 14c2c41)
@mergify mergify bot mentioned this pull request Jun 27, 2023
Merged
mergify bot pushed a commit that referenced this pull request Jun 27, 2023
@JanStaschulat @mergify
rclc_executor: improve enum type names (#379)
fe2db05 
(cherry picked from commit 14c2c41)

# Conflicts:
#	rclc/include/rclc/executor.h
@mergify mergify bot mentioned this pull request Jun 27, 2023
Closed
@gavanderhoorn
Copy link

Thank you @JanStaschulat 👍

JanStaschulat pushed a commit that referenced this pull request Jun 27, 2023
@mergify
rclc_executor: improve enum type names (#379) (#382)
24ab126
@gavanderhoorn gavanderhoorn mentioned this pull request Jun 27, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gavanderhoorn gavanderhoorn gavanderhoorn approved these changes

@pablogs9 pablogs9 pablogs9 approved these changes

Assignees

@JanStaschulat JanStaschulat

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@JanStaschulat @gavanderhoorn @codecov-commenter @pablogs9